/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Needs one bug fix before merging — success message has swapped arguments; everything else looks solid.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_pipeline_artifact.rs:524–525 — effective_build_id and final_name are swapped in the success_with_data format string. The info!() log four lines above has them in the correct order; the returned ExecutionResult message does not.

  // Current (wrong) — produces: "Published 'out/report.pdf' as pipeline artifact '42' on build #my-artifact"
  format!("Published '{}' as pipeline artifact '{}' on build #{}", self.file_path, effective_build_id, final_name)
  
  // Correct
  format!("Published '{}' as pipeline artifact '{}' on build #{}", self.file_path, final_name, effective_build_id)

  The structured JSON data in the same success_with_data call is correct, so the artifact upload itself works — only the human-readable summary is garbled.

⚠️ Suggestions

• src/mcp.rs (upload_pipeline_artifact handler) — file_size is captured from tokio::fs::metadata before the file is read. If the source file is modified in the narrow window between stat and read, the stored file_size will not match the bytes actually staged. The Stage 3 size check would then produce a confusing "file was modified between stages" failure even though the staged content is internally consistent (SHA-256 covers the actual bytes). Using source_bytes.len() as u64 instead of metadata.len() would make the stored size match the staged content exactly. The upload-build-attachment handler has the same pattern — worth fixing in both.

✅ What Looks Good

• Symlink-escape prevention is correct in both the MCP handler (canonicalize + starts_with check) and the Stage 3 executor.
• SHA-256 integrity chain (hash at Stage 1, verify at Stage 3 before reading) is solid and matches the upload-build-attachment pattern.
• Build-ID allow-list correctly skips the check when build_id is None (current build), matching the documented intent.
• URL construction uses utf8_percent_encode consistently for all user-controlled path segments — no injection risk.
• The rename from upload-build-artifact → upload-build-attachment is thorough: dispatch table, NDJSON names, log strings, staged filename prefix, and all re-exports are updated.

Generated by Rust PR Reviewer for issue #404 · ● 861.6K · ◷

🔍 Rust PR Review

Summary: Looks good overall — the rename is clean and the new upload-pipeline-artifact tool follows established patterns well. One correctness concern in the ADO API call worth verifying, and the breaking change needs signaling.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_pipeline_artifact.rs — Step 1 container creation body is missing scope

  The container creation POST sends only {"name": final_name}. The ADO containers API (/_apis/resources/containers) scopes containers to a project via the scope field in the request body (the project GUID). Without it the container is created at the collection scope. Step 2 then uploads with scope={project_id} as a query param — but the container was created unscoped, which can cause the upload or the Step 3 association to fail depending on the ADO instance version. The fix is to include the project GUID in the body:

  let container_body = serde_json::json!({
      "name": final_name,
      "scope": project_id,   // <-- add this
  });

  The project_id variable is already resolved earlier in the function.

• Breaking change not signaled in PR title

  Removing upload-build-artifact (including from WRITE_REQUIRING_SAFE_OUTPUTS and ALL_KNOWN_SAFE_OUTPUTS) is a breaking change for any consumer that has upload-build-artifact in their front-matter or execute dispatch. The PR title uses feat(safe-outputs): rather than feat!(safe-outputs):, so release-please will generate a minor bump rather than a major one. Per the project's Conventional Commits convention, a BREAKING CHANGE: footer or a ! suffix on the type is required.

✅ What Looks Good

• TOCTOU fix is solid — using source_bytes.len() as u64 instead of metadata.len() to record the staged size in both MCP handlers closes the race window correctly without any additional code.
• Path traversal protection — both canonicalize() + starts_with(canonical_root) checks are present and consistent with the existing upload-build-attachment pattern.
• SHA-256 integrity chain — hash recorded at Stage 1 is verified at Stage 3 after reading the bytes (not from metadata), which is the right place to detect tampering.
• URL encoding — all user-controlled values in the three REST calls are wrapped in utf8_percent_encode(..., PATH_SEGMENT) before being placed in URLs.
• ado_project_id wiring — the SYSTEM_TEAMPROJECTID env var is cleanly added to ExecutionContext and all existing test contexts are updated with ado_project_id: None.
• Test coverage — param validation, dry-run, size mismatch, max-size rejection, build-id allow-list, and extension allow-list are all exercised.

Generated by Rust PR Reviewer for issue #404 · ● 953.6K · ◷

🔍 Rust PR Review

Summary: Looks good overall — the implementation is solid and follows project conventions well. One notable test coverage gap deserves follow-up.

Findings

⚠️ Suggestions

• src/safeoutputs/upload_pipeline_artifact.rs — missing SHA-256 tamper tests
  upload_build_attachment.rs has both test_executor_rejects_tampered_staged_file and test_executor_rejects_sha256_mismatch (covering the case where the file content changes between stages). upload_pipeline_artifact.rs has neither. Since the SHA-256 integrity check is the primary security control preventing Stage 1→3 tampering, this path should be explicitly exercised. Consider adding equivalent tests before any future changes touch execute_impl.

• src/safeoutputs/upload_pipeline_artifact.rs:285 — allowed-artifact-names applies to the post-prefix name
  The check runs against final_name (after name-prefix is prepended), consistent with upload_build_attachment. But the docs/safe-outputs.md description for the new tool just says "restrict names (suffix * = prefix match)" without clarifying this. An operator who sets name-prefix: "ci-" and allowed-artifact-names: ["report"] will find the allowlist never matches. Worth a documentation clarification (e.g., "patterns match the final artifact name, including any configured name-prefix").

• src/safeoutputs/upload_pipeline_artifact.rs — name-prefix charset not validated at config-load time
  Only the length (max 50) is checked; invalid characters are only caught at execute time via is_valid_artifact_name(&final_name), which produces a generic "Resolved artifact name ... is not a valid ADO artifact name" error. Low priority since this is operator config, not agent input — but matching the build_attachment pattern of an early explicit check would improve diagnosability.

✅ What Looks Good

• TOCTOU window properly closed: reading bytes with tokio::fs::read and recording source_bytes.len() as file_size (rather than the earlier metadata.len()) is exactly right, and the added comment explains why.
• Double path-traversal defense: canonicalize + starts_with bounding-directory check at Stage 1 (MCP) and again at Stage 3 (executor) — correct defense-in-depth.
• SHA-256 integrity: the staged-file hash is checked after tokio::fs::read in Stage 3, so any tampering between stages is detected regardless of the metadata size check ordering.
• Backward compatibility handled cleanly: the rename is complete — no leftover upload-build-artifact references in source, docs, or tests.
• ado_project_id wiring: correctly added to ExecutionContext from SYSTEM_TEAMPROJECTID and surfaced with a clear .context(...) error message when absent.

Generated by Rust PR Reviewer for issue #404 · ● 1.4M · ◷